-
Notifications
You must be signed in to change notification settings - Fork 13
Cleanup public interface #589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
@matentzn @joeflack4 I'd appreciate a review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cthoyt Thanks for adding these aliases! Other misc changes and docs look good, too.
Just one more thing on my wishlist: method/func names and locations
tests/test_parsers.py
Outdated
msdf = parse_sssom_table(stream) | ||
input_path = test_data_dir.joinpath("basic.tsv") | ||
with input_path.open() as file: | ||
msdf = parse_sssom_table(file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the intention of simplifying the code here (no need to read the entire contents of basic.tsv
into a string, turn the string into a file-like StringIO
object, and then pass that object to parse_sssom_table
, when that function can take a pathname directly), but in this instance I believe this is ill-advised.
Judging from the name of this test (test_parse_sssom_dataframe_from_stringio
), it is specifically intended to check that you can use parse_sssom_table
on a StringIO
object, rather than a file descriptor or a pathname. Now it is no longer testing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, so I updated this in aabdb93.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matentzn done |
This PR does the following:
parse_tsv()
andparse_csv()
as aliases forparse_sssom_table
that don't try and guess the right separator, which makes it easier to understand exactly what the function is forsssom.parse_tsv()
instead of needing to know it's hiding insssom.writers.parse_tsv
sssom.parse
function which just wrapspd.read_csv()
, and is only used by a testDemo